Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Clang] Reapply CWG2369 "Ordering between constraints and substitution" #122423

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

zyn0217
Copy link
Contributor

@zyn0217 zyn0217 commented Jan 10, 2025

The previous approach broke code generation for the MS ABI due to an unintended code path during constraint substitution. This time we address the issue by inspecting the evaluation contexts and thereby avoiding that code path.

This reapplies 96eced6 (#102857).

@zyn0217 zyn0217 requested a review from cor3ntin January 10, 2025 05:40
@zyn0217 zyn0217 requested a review from Endilll as a code owner January 10, 2025 05:40
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 10, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 10, 2025

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

The previous approach broke code generation for the MS ABI due to an unintended code path during constraint substitution. This time we address the issue by inspecting the CodeSynthesisContexts and thereby avoiding that code path.

This reapplies 96eced6.


Patch is 41.40 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/122423.diff

25 Files Affected:

  • (modified) clang/include/clang/Sema/Sema.h (+26-5)
  • (modified) clang/include/clang/Sema/Template.h (+6)
  • (modified) clang/lib/Sema/Sema.cpp (+2-1)
  • (modified) clang/lib/Sema/SemaConcept.cpp (+45-2)
  • (modified) clang/lib/Sema/SemaTemplateDeduction.cpp (+35-14)
  • (modified) clang/lib/Sema/SemaTemplateDeductionGuide.cpp (+5-3)
  • (modified) clang/lib/Sema/SemaTemplateInstantiate.cpp (+106-11)
  • (modified) clang/lib/Sema/SemaTemplateInstantiateDecl.cpp (+7-2)
  • (modified) clang/lib/Sema/TreeTransform.h (+1-1)
  • (modified) clang/test/CXX/drs/cwg23xx.cpp (+29)
  • (modified) clang/test/CXX/drs/cwg26xx.cpp (+1-1)
  • (modified) clang/test/CXX/drs/cwg27xx.cpp (+20)
  • (modified) clang/test/CXX/expr/expr.prim/expr.prim.req/nested-requirement.cpp (+1-1)
  • (modified) clang/test/CXX/temp/temp.constr/temp.constr.atomic/constrant-satisfaction-conversions.cpp (+3-3)
  • (added) clang/test/CodeGenCXX/ms-mangle-requires.cpp (+29)
  • (modified) clang/test/SemaCXX/concept-crash-on-diagnostic.cpp (+1-1)
  • (modified) clang/test/SemaCXX/cxx20-ctad-type-alias.cpp (+1-1)
  • (modified) clang/test/SemaCXX/cxx23-assume.cpp (+3-3)
  • (modified) clang/test/SemaCXX/cxx2c-fold-exprs.cpp (+1-1)
  • (modified) clang/test/SemaCXX/lambda-unevaluated.cpp (+2-2)
  • (modified) clang/test/SemaTemplate/concepts-recursive-inst.cpp (+2-2)
  • (modified) clang/test/SemaTemplate/cxx2a-constraint-exprs.cpp (+1-1)
  • (modified) clang/test/SemaTemplate/deduction-guide.cpp (-5)
  • (modified) clang/test/SemaTemplate/nested-implicit-deduction-guides.cpp (+5-3)
  • (modified) clang/www/cxx_dr_status.html (+6-2)
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index a41f16f6dc8c9b..87d9a335763e31 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -13062,6 +13062,7 @@ class Sema final : public SemaBase {
   ///
   /// \param SkipForSpecialization when specified, any template specializations
   /// in a traversal would be ignored.
+  ///
   /// \param ForDefaultArgumentSubstitution indicates we should continue looking
   /// when encountering a specialized member function template, rather than
   /// returning immediately.
@@ -13073,6 +13074,17 @@ class Sema final : public SemaBase {
       bool SkipForSpecialization = false,
       bool ForDefaultArgumentSubstitution = false);
 
+  /// Apart from storing the result to \p Result, this behaves the same as
+  /// another overload.
+  void getTemplateInstantiationArgs(
+      MultiLevelTemplateArgumentList &Result, const NamedDecl *D,
+      const DeclContext *DC = nullptr, bool Final = false,
+      std::optional<ArrayRef<TemplateArgument>> Innermost = std::nullopt,
+      bool RelativeToPrimary = false, const FunctionDecl *Pattern = nullptr,
+      bool ForConstraintInstantiation = false,
+      bool SkipForSpecialization = false,
+      bool ForDefaultArgumentSubstitution = false);
+
   /// RAII object to handle the state changes required to synthesize
   /// a function body.
   class SynthesizedFunctionScope {
@@ -13169,6 +13181,10 @@ class Sema final : public SemaBase {
   // FIXME: Should we have a similar limit for other forms of synthesis?
   unsigned NonInstantiationEntries;
 
+  /// The number of \p CodeSynthesisContexts that are not constraint
+  /// substitution.
+  unsigned NonConstraintSubstitutionEntries;
+
   /// The depth of the context stack at the point when the most recent
   /// error or warning was produced.
   ///
@@ -13342,7 +13358,7 @@ class Sema final : public SemaBase {
   ExprResult
   SubstConstraintExpr(Expr *E,
                       const MultiLevelTemplateArgumentList &TemplateArgs);
-  // Unlike the above, this does not evaluates constraints.
+  // Unlike the above, this does not evaluate constraints.
   ExprResult SubstConstraintExprWithoutSatisfaction(
       Expr *E, const MultiLevelTemplateArgumentList &TemplateArgs);
 
@@ -13491,6 +13507,11 @@ class Sema final : public SemaBase {
     return CodeSynthesisContexts.size() > NonInstantiationEntries;
   }
 
+  /// Determine whether we are currently performing constraint substitution.
+  bool inConstraintSubstitution() const {
+    return CodeSynthesisContexts.size() > NonConstraintSubstitutionEntries;
+  }
+
   using EntityPrinter = llvm::function_ref<void(llvm::raw_ostream &)>;
 
   /// \brief create a Requirement::SubstitutionDiagnostic with only a
@@ -14463,10 +14484,10 @@ class Sema final : public SemaBase {
       const MultiLevelTemplateArgumentList &TemplateArgs,
       SourceRange TemplateIDRange);
 
-  bool CheckInstantiatedFunctionTemplateConstraints(
-      SourceLocation PointOfInstantiation, FunctionDecl *Decl,
-      ArrayRef<TemplateArgument> TemplateArgs,
-      ConstraintSatisfaction &Satisfaction);
+  bool CheckFunctionTemplateConstraints(SourceLocation PointOfInstantiation,
+                                        FunctionDecl *Decl,
+                                        ArrayRef<TemplateArgument> TemplateArgs,
+                                        ConstraintSatisfaction &Satisfaction);
 
   /// \brief Emit diagnostics explaining why a constraint expression was deemed
   /// unsatisfied.
diff --git a/clang/include/clang/Sema/Template.h b/clang/include/clang/Sema/Template.h
index 9800f75f676aaf..59a0575ca98036 100644
--- a/clang/include/clang/Sema/Template.h
+++ b/clang/include/clang/Sema/Template.h
@@ -522,6 +522,12 @@ enum class TemplateSubstitutionKind : char {
     llvm::PointerUnion<Decl *, DeclArgumentPack *> *
     findInstantiationOf(const Decl *D);
 
+    /// Similar to \p findInstantiationOf(), but it wouldn't assert if the
+    /// instantiation was not found within the current instantiation scope. This
+    /// is helpful for on-demand declaration instantiation.
+    llvm::PointerUnion<Decl *, DeclArgumentPack *> *
+    findInstantiationUnsafe(const Decl *D);
+
     void InstantiatedLocal(const Decl *D, Decl *Inst);
     void InstantiatedLocalPackArg(const Decl *D, VarDecl *Inst);
     void MakeInstantiatedLocalArgPack(const Decl *D);
diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp
index abb46d3a84e74e..b7b154e55b3db9 100644
--- a/clang/lib/Sema/Sema.cpp
+++ b/clang/lib/Sema/Sema.cpp
@@ -263,7 +263,8 @@ Sema::Sema(Preprocessor &pp, ASTContext &ctxt, ASTConsumer &consumer,
       TyposCorrected(0), IsBuildingRecoveryCallExpr(false), NumSFINAEErrors(0),
       AccessCheckingSFINAE(false), CurrentInstantiationScope(nullptr),
       InNonInstantiationSFINAEContext(false), NonInstantiationEntries(0),
-      ArgumentPackSubstitutionIndex(-1), SatisfactionCache(Context) {
+      NonConstraintSubstitutionEntries(0), ArgumentPackSubstitutionIndex(-1),
+      SatisfactionCache(Context) {
   assert(pp.TUKind == TUKind);
   TUScope = nullptr;
 
diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp
index 539de00bd104f5..10f4920a761f3c 100644
--- a/clang/lib/Sema/SemaConcept.cpp
+++ b/clang/lib/Sema/SemaConcept.cpp
@@ -846,7 +846,7 @@ bool Sema::CheckFunctionConstraints(const FunctionDecl *FD,
                                     bool ForOverloadResolution) {
   // Don't check constraints if the function is dependent. Also don't check if
   // this is a function template specialization, as the call to
-  // CheckinstantiatedFunctionTemplateConstraints after this will check it
+  // CheckFunctionTemplateConstraints after this will check it
   // better.
   if (FD->isDependentContext() ||
       FD->getTemplatedKind() ==
@@ -1111,12 +1111,55 @@ bool Sema::EnsureTemplateArgumentListConstraints(
   return false;
 }
 
-bool Sema::CheckInstantiatedFunctionTemplateConstraints(
+static bool CheckFunctionConstraintsWithoutInstantiation(
+    Sema &SemaRef, SourceLocation PointOfInstantiation,
+    FunctionTemplateDecl *Template, ArrayRef<TemplateArgument> TemplateArgs,
+    ConstraintSatisfaction &Satisfaction) {
+  SmallVector<const Expr *, 3> TemplateAC;
+  Template->getAssociatedConstraints(TemplateAC);
+  if (TemplateAC.empty()) {
+    Satisfaction.IsSatisfied = true;
+    return false;
+  }
+
+  LocalInstantiationScope Scope(SemaRef);
+
+  FunctionDecl *FD = Template->getTemplatedDecl();
+  // Collect the list of template arguments relative to the 'primary'
+  // template. We need the entire list, since the constraint is completely
+  // uninstantiated at this point.
+
+  // FIXME: Add TemplateArgs through the 'Innermost' parameter once
+  // the refactoring of getTemplateInstantiationArgs() relands.
+  MultiLevelTemplateArgumentList MLTAL;
+  MLTAL.addOuterTemplateArguments(Template, std::nullopt, /*Final=*/false);
+  SemaRef.getTemplateInstantiationArgs(
+      MLTAL, /*D=*/FD, FD,
+      /*Final=*/false, /*Innermost=*/std::nullopt, /*RelativeToPrimary=*/true,
+      /*Pattern=*/nullptr, /*ForConstraintInstantiation=*/true);
+  MLTAL.replaceInnermostTemplateArguments(Template, TemplateArgs);
+
+  Sema::ContextRAII SavedContext(SemaRef, FD);
+  std::optional<Sema::CXXThisScopeRAII> ThisScope;
+  if (auto *Method = dyn_cast<CXXMethodDecl>(FD))
+    ThisScope.emplace(SemaRef, /*Record=*/Method->getParent(),
+                      /*ThisQuals=*/Method->getMethodQualifiers());
+  return SemaRef.CheckConstraintSatisfaction(
+      Template, TemplateAC, MLTAL, PointOfInstantiation, Satisfaction);
+}
+
+bool Sema::CheckFunctionTemplateConstraints(
     SourceLocation PointOfInstantiation, FunctionDecl *Decl,
     ArrayRef<TemplateArgument> TemplateArgs,
     ConstraintSatisfaction &Satisfaction) {
   // In most cases we're not going to have constraints, so check for that first.
   FunctionTemplateDecl *Template = Decl->getPrimaryTemplate();
+
+  if (!Template)
+    return ::CheckFunctionConstraintsWithoutInstantiation(
+        *this, PointOfInstantiation, Decl->getDescribedFunctionTemplate(),
+        TemplateArgs, Satisfaction);
+
   // Note - code synthesis context for the constraints check is created
   // inside CheckConstraintsSatisfaction.
   SmallVector<const Expr *, 3> TemplateAC;
diff --git a/clang/lib/Sema/SemaTemplateDeduction.cpp b/clang/lib/Sema/SemaTemplateDeduction.cpp
index 1c1f6e30ab7b83..acd1151184e42f 100644
--- a/clang/lib/Sema/SemaTemplateDeduction.cpp
+++ b/clang/lib/Sema/SemaTemplateDeduction.cpp
@@ -3936,18 +3936,6 @@ TemplateDeductionResult Sema::FinishTemplateArgumentDeduction(
       Result != TemplateDeductionResult::Success)
     return Result;
 
-  // C++ [temp.deduct.call]p10: [DR1391]
-  //   If deduction succeeds for all parameters that contain
-  //   template-parameters that participate in template argument deduction,
-  //   and all template arguments are explicitly specified, deduced, or
-  //   obtained from default template arguments, remaining parameters are then
-  //   compared with the corresponding arguments. For each remaining parameter
-  //   P with a type that was non-dependent before substitution of any
-  //   explicitly-specified template arguments, if the corresponding argument
-  //   A cannot be implicitly converted to P, deduction fails.
-  if (CheckNonDependent())
-    return TemplateDeductionResult::NonDependentConversionFailure;
-
   // Form the template argument list from the deduced template arguments.
   TemplateArgumentList *SugaredDeducedArgumentList =
       TemplateArgumentList::CreateCopy(Context, SugaredBuilder);
@@ -3977,6 +3965,39 @@ TemplateDeductionResult Sema::FinishTemplateArgumentDeduction(
     FD = const_cast<FunctionDecl *>(FDFriend);
     Owner = FD->getLexicalDeclContext();
   }
+  // C++20 [temp.deduct.general]p5: [CWG2369]
+  //   If the function template has associated constraints, those constraints
+  //   are checked for satisfaction. If the constraints are not satisfied, type
+  //   deduction fails.
+  //
+  // FIXME: We haven't implemented CWG2369 for lambdas yet, because we need
+  // to figure out how to instantiate lambda captures to the scope without
+  // first instantiating the lambda.
+  bool IsLambda = isLambdaCallOperator(FD) || isLambdaConversionOperator(FD);
+  if (!IsLambda && !IsIncomplete) {
+    if (CheckFunctionTemplateConstraints(
+            Info.getLocation(),
+            FunctionTemplate->getCanonicalDecl()->getTemplatedDecl(),
+            CanonicalBuilder, Info.AssociatedConstraintsSatisfaction))
+      return TemplateDeductionResult::MiscellaneousDeductionFailure;
+    if (!Info.AssociatedConstraintsSatisfaction.IsSatisfied) {
+      Info.reset(Info.takeSugared(),
+                 TemplateArgumentList::CreateCopy(Context, CanonicalBuilder));
+      return TemplateDeductionResult::ConstraintsNotSatisfied;
+    }
+  }
+  // C++ [temp.deduct.call]p10: [CWG1391]
+  //   If deduction succeeds for all parameters that contain
+  //   template-parameters that participate in template argument deduction,
+  //   and all template arguments are explicitly specified, deduced, or
+  //   obtained from default template arguments, remaining parameters are then
+  //   compared with the corresponding arguments. For each remaining parameter
+  //   P with a type that was non-dependent before substitution of any
+  //   explicitly-specified template arguments, if the corresponding argument
+  //   A cannot be implicitly converted to P, deduction fails.
+  if (CheckNonDependent())
+    return TemplateDeductionResult::NonDependentConversionFailure;
+
   MultiLevelTemplateArgumentList SubstArgs(
       FunctionTemplate, CanonicalDeducedArgumentList->asArray(),
       /*Final=*/false);
@@ -4011,8 +4032,8 @@ TemplateDeductionResult Sema::FinishTemplateArgumentDeduction(
   //   ([temp.constr.decl]), those constraints are checked for satisfaction
   //   ([temp.constr.constr]). If the constraints are not satisfied, type
   //   deduction fails.
-  if (!IsIncomplete) {
-    if (CheckInstantiatedFunctionTemplateConstraints(
+  if (IsLambda && !IsIncomplete) {
+    if (CheckFunctionTemplateConstraints(
             Info.getLocation(), Specialization, CanonicalBuilder,
             Info.AssociatedConstraintsSatisfaction))
       return TemplateDeductionResult::MiscellaneousDeductionFailure;
diff --git a/clang/lib/Sema/SemaTemplateDeductionGuide.cpp b/clang/lib/Sema/SemaTemplateDeductionGuide.cpp
index d42c3765aa534f..5d6c11a75303df 100644
--- a/clang/lib/Sema/SemaTemplateDeductionGuide.cpp
+++ b/clang/lib/Sema/SemaTemplateDeductionGuide.cpp
@@ -902,10 +902,12 @@ Expr *buildIsDeducibleConstraint(Sema &SemaRef,
       Context.getTrivialTypeSourceInfo(
           Context.getDeducedTemplateSpecializationType(
               TemplateName(AliasTemplate), /*DeducedType=*/QualType(),
-              /*IsDependent=*/true)), // template specialization type whose
-                                      // arguments will be deduced.
+              /*IsDependent=*/true),
+          AliasTemplate->getLocation()), // template specialization type whose
+                                         // arguments will be deduced.
       Context.getTrivialTypeSourceInfo(
-          ReturnType), // type from which template arguments are deduced.
+          ReturnType, AliasTemplate->getLocation()), // type from which template
+                                                     // arguments are deduced.
   };
   return TypeTraitExpr::Create(
       Context, Context.getLogicalOperationType(), AliasTemplate->getLocation(),
diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index fb0f38df62a744..6db6d52e27d6dc 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -475,6 +475,21 @@ MultiLevelTemplateArgumentList Sema::getTemplateInstantiationArgs(
   assert((ND || DC) && "Can't find arguments for a decl if one isn't provided");
   // Accumulate the set of template argument lists in this structure.
   MultiLevelTemplateArgumentList Result;
+  getTemplateInstantiationArgs(
+      Result, ND, DC, Final, Innermost, RelativeToPrimary, Pattern,
+      ForConstraintInstantiation, SkipForSpecialization,
+      ForDefaultArgumentSubstitution);
+  return Result;
+}
+
+void Sema::getTemplateInstantiationArgs(
+    MultiLevelTemplateArgumentList &Result, const NamedDecl *ND,
+    const DeclContext *DC, bool Final,
+    std::optional<ArrayRef<TemplateArgument>> Innermost, bool RelativeToPrimary,
+    const FunctionDecl *Pattern, bool ForConstraintInstantiation,
+    bool SkipForSpecialization, bool ForDefaultArgumentSubstitution) {
+  assert((ND || DC) && "Can't find arguments for a decl if one isn't provided");
+  // Accumulate the set of template argument lists in this structure.
 
   using namespace TemplateInstArgsHelpers;
   const Decl *CurDecl = ND;
@@ -535,14 +550,12 @@ MultiLevelTemplateArgumentList Sema::getTemplateInstantiationArgs(
     }
 
     if (R.IsDone)
-      return Result;
+      return;
     if (R.ClearRelativeToPrimary)
       RelativeToPrimary = false;
     assert(R.NextDecl);
     CurDecl = R.NextDecl;
   }
-
-  return Result;
 }
 
 bool Sema::CodeSynthesisContext::isInstantiationRecord() const {
@@ -815,6 +828,9 @@ void Sema::pushCodeSynthesisContext(CodeSynthesisContext Ctx) {
   if (!Ctx.isInstantiationRecord())
     ++NonInstantiationEntries;
 
+  if (Ctx.Kind != CodeSynthesisContext::ConstraintSubstitution)
+    ++NonConstraintSubstitutionEntries;
+
   // Check to see if we're low on stack space. We can't do anything about this
   // from here, but we can at least warn the user.
   StackHandler.warnOnStackNearlyExhausted(Ctx.PointOfInstantiation);
@@ -827,6 +843,11 @@ void Sema::popCodeSynthesisContext() {
     --NonInstantiationEntries;
   }
 
+  if (Active.Kind != CodeSynthesisContext::ConstraintSubstitution) {
+    assert(NonConstraintSubstitutionEntries > 0);
+    --NonConstraintSubstitutionEntries;
+  }
+
   InNonInstantiationSFINAEContext = Active.SavedInNonInstantiationSFINAEContext;
 
   // Name lookup no longer looks in this template's defining module.
@@ -1349,6 +1370,12 @@ namespace {
     // Whether an incomplete substituion should be treated as an error.
     bool BailOutOnIncomplete;
 
+  private:
+    // CWG2770: Function parameters should be instantiated when they are
+    // needed by a satisfaction check of an atomic constraint or
+    // (recursively) by another function parameter.
+    bool maybeInstantiateFunctionParameterToScope(ParmVarDecl *OldParm);
+
   public:
     typedef TreeTransform<TemplateInstantiator> inherited;
 
@@ -1405,12 +1432,20 @@ namespace {
                                  ArrayRef<UnexpandedParameterPack> Unexpanded,
                                  bool &ShouldExpand, bool &RetainExpansion,
                                  std::optional<unsigned> &NumExpansions) {
-      return getSema().CheckParameterPacksForExpansion(EllipsisLoc,
-                                                       PatternRange, Unexpanded,
-                                                       TemplateArgs,
-                                                       ShouldExpand,
-                                                       RetainExpansion,
-                                                       NumExpansions);
+      if (SemaRef.CurrentInstantiationScope &&
+          SemaRef.inConstraintSubstitution()) {
+        for (UnexpandedParameterPack ParmPack : Unexpanded) {
+          NamedDecl *VD = ParmPack.first.dyn_cast<NamedDecl *>();
+          if (!isa_and_present<ParmVarDecl>(VD))
+            continue;
+          if (maybeInstantiateFunctionParameterToScope(cast<ParmVarDecl>(VD)))
+            return true;
+        }
+      }
+
+      return getSema().CheckParameterPacksForExpansion(
+          EllipsisLoc, PatternRange, Unexpanded, TemplateArgs, ShouldExpand,
+          RetainExpansion, NumExpansions);
     }
 
     void ExpandingFunctionParameterPack(ParmVarDecl *Pack) {
@@ -1911,9 +1946,62 @@ Decl *TemplateInstantiator::TransformDecl(SourceLocation Loc, Decl *D) {
     // template parameter.
   }
 
+  if (SemaRef.CurrentInstantiationScope) {
+    if (SemaRef.inConstraintSubstitution() && isa<ParmVarDecl>(D) &&
+        maybeInstantiateFunctionParameterToScope(cast<ParmVarDecl>(D)))
+      return nullptr;
+  }
+
   return SemaRef.FindInstantiatedDecl(Loc, cast<NamedDecl>(D), TemplateArgs);
 }
 
+bool TemplateInstantiator::maybeInstantiateFunctionParameterToScope(
+    ParmVarDecl *OldParm) {
+  if (SemaRef.CurrentInstantiationScope->findInstantiationUnsafe(OldParm))
+    return false;
+  // We're instantiating a function parameter whose associated function template
+  // has not been instantiated at this point for constraint evaluation, so make
+  // sure the instantiated parameters are owned by a function declaration such
+  // that they can be correctly 'captured' in tryCaptureVariable().
+  Sema::ContextRAII Context(SemaRef, OldParm->getDeclContext());
+
+  if (!OldParm->isParameterPack())
+    return !TransformFunctionTypeParam(OldParm, /*indexAdjustment=*/0,
+                                       /*NumExpansions=*/std::nullopt,
+                                       /*ExpectParameterPack=*/false);
+
+  SmallVector<UnexpandedParameterPack, 2> Unexpanded;
+
+  // Find the parameter packs that could be expanded.
+  TypeLoc TL = OldParm->getTypeSourceInfo()->getTypeLoc();
+  PackExpansionTypeLoc ExpansionTL = TL.castAs<PackExpansionTypeLoc>();
+  TypeLoc Pattern = ExpansionTL.getPatternLoc();
+  SemaRef.collectUnexpandedParameterPacks(Pattern, Unexpanded);
+  assert(!Unexpanded.empty() && "Pack expansion without parameter packs?");
+
+  bool ShouldExpand = false;
+  bool RetainExpansion = false;
+  std::optional<unsigned> OrigNumExpansions =
+      ExpansionTL.getTypePtr()->getNumExpansions();
+  std::optional<unsigned> NumExpansions = OrigNumExpansions;
+  if (TryExpandParameterPacks(ExpansionTL.getEllipsisLoc(),
+                              Pattern.getSourceRange(), Unexpanded,
+                              ShouldExpand, RetainExpansion, NumExpansions))
+    return true;
+
+  assert(ShouldExpand && !RetainExpansion &&
+         "Shouldn't preserve pack expansion when evaluating constraints");
+  ExpandingFunctionParameterPack(OldParm);
+  ...
[truncated]

@zyn0217 zyn0217 marked this pull request as draft January 10, 2025 08:57
@zyn0217 zyn0217 force-pushed the cwg-2369-again branch 3 times, most recently from 375253d to 65829ef Compare January 10, 2025 09:14
@zyn0217 zyn0217 marked this pull request as ready for review January 11, 2025 08:16
@zyn0217 zyn0217 marked this pull request as draft January 11, 2025 14:20
@zyn0217 zyn0217 marked this pull request as ready for review January 11, 2025 14:55
Comment on lines +5287 to +5289
// With CWG2369, we substitute constraints before instantiating the associated
// function template. This helps prevent potential code generation for
// dependent types, particularly under the MS ABI.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// With CWG2369, we substitute constraints before instantiating the associated
// function template. This helps prevent potential code generation for
// dependent types, particularly under the MS ABI.
// We never need to emit the code for a lambda in unevaluated context.
// We also can't mangle a lambda in the require clause of a function template
// during constraint checking as the MSI ABI would need to mangle the (not yet specialized) enclosing declaration
// FIXME: Should we try to skip this for non-lambda functions too?

@cor3ntin
Copy link
Contributor

I've confirmed with @zyn0217 that this iteration of the patch fixes the bug encountered by the chromium team wherein we tried to mangle lambdas with a deduced return type during constraint satisfaction - which caused failure on windows as mangling of lambda requires the enclosing context to be mangled.


However if we merge this PR, the bug described in #121881
would be reintroduced.

The issue is that users seem to rely on a GCC specific behavior where GCC does not check template candidates during overload resolution if an exact match exists.
This is a known divergence that is known to both Clang and WG21 (see #62096)

Given that GCC's behavior is strictly non-conforming but long standing, I have asked CWG for guidance
https://lists.isocpp.org/core/2025/01/16814.php

That being said, I think our option are

  • Wait for WG21 to decide what to do and implement whatever resolution they come up with (which may or not be a breaking change for us)

  • Follow GCC's behavior immediately, however there are some drawbacks as it seems like users which relies on it do so inadvertently (and some diagnostics would not be emitted)

  • Only implement this DR under pedantic or c++26 mode to limit breakage

  • Accept the breakage

@zygoloid @hubert-reinterpretcast @erichkeane @AaronBallman

@zyn0217
Copy link
Contributor Author

zyn0217 commented Jan 12, 2025

@mysterymath @zmodem Can you both test this patch on Windows to confirm if it resolves issues you've reported? We'd greatly appreciate it!

@alexfh
Copy link
Contributor

alexfh commented Jan 13, 2025

FYI, we've also seen problems compiling code that includes certain boost headers (version 1.84, as far as I can tell) after #102857:

boost/asio/any_io_executor.hpp:161:13: error: satisfaction of constraint '::boost::asio::execution::executor<Executor>' depends on itself
  161 |   template <BOOST_ASIO_EXECUTION_EXECUTOR Executor>
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
...

I'm building clang with this PR to see if it resolves this issue.

@zmodem
Copy link
Collaborator

zmodem commented Jan 13, 2025

Confirmed Chromium on Windows builds with this patch.

@cor3ntin
Copy link
Contributor

@alexfh It wouldn't fix your issue, see this comment #122423 (comment) (there were 2 issues, a bug in the PR implementation, and an unfortunate breakage caused by some libraries relying on non-conforming behavior)

@alexfh
Copy link
Contributor

alexfh commented Jan 14, 2025

@alexfh It wouldn't fix your issue, see this comment #122423 (comment) (there were 2 issues, a bug in the PR implementation, and an unfortunate breakage caused by some libraries relying on non-conforming behavior)

Indeed, it doesn't fix this case. Neither does it fix another compilation error in the same version of boost:

boost/asio/detail/handler_work.hpp:452:7: error: ambiguous partial specializations of 'handler_work_base<boost::asio::any_io_executor>'
  452 |   > : handler_work_base<IoExecutor>
      |       ^

@Endilll
Copy link
Contributor

Endilll commented Jan 15, 2025

This was discussed during Clang C/C++ Language Working Group meeting today, and the consensus seems that this PR shouldn't land before Clang 20 branch, which is scheduled to happen on January 28th.

@alexfh
Copy link
Contributor

alexfh commented Jan 21, 2025

Can we have this behavior guarded by a compiler option?

@hubert-reinterpretcast
Copy link
Collaborator

Can we have this behavior guarded by a compiler option?

That depends on the rationale:

  1. Allow opt in to the resolution of CWG2369 because it is wanted (despite potentially-temporary fallout); or
  2. Require opt in to the resolution of CWG2369 because "it causes fallout".

For (2), we can simply delay enabling the resolution until the fallout is mitigated or accepted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants